Skip to content

stream: simplify setPromiseHandled utility#63032

Open
aduh95 wants to merge 1 commit intonodejs:mainfrom
aduh95:simpler-setPromiseHandled
Open

stream: simplify setPromiseHandled utility#63032
aduh95 wants to merge 1 commit intonodejs:mainfrom
aduh95:simpler-setPromiseHandled

Conversation

@aduh95
Copy link
Copy Markdown
Contributor

@aduh95 aduh95 commented Apr 29, 2026

We only need a onRejected value to mark the promise as handled, having an empty onFulfilled might help the JS engine to better optimize this code path (though I doubt it's measurable).

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. web streams labels Apr 29, 2026
@targos
Copy link
Copy Markdown
Member

targos commented Apr 29, 2026

What about using PromisePrototypeCatch ?

@aduh95
Copy link
Copy Markdown
Contributor Author

aduh95 commented Apr 29, 2026 •
edited
Loading

What about using PromisePrototypeCatch ?

node/tools/eslint-rules/avoid-prototype-pollution.js

Lines 204 to 210 in 1f9636a

[CallExpression('PromisePrototypeCatch')](node) {
context.report({
node,
message: '%Promise.prototype.catch% looks up the `then` property of ' +
'the `this` argument, use PromisePrototypeThen instead',
});
},

@aduh95
stream: simplify setPromiseHandled utility
c70ec6d 
Signed-off-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@aduh95 aduh95 force-pushed the simpler-setPromiseHandled branch from 78b154b to c70ec6d Compare April 29, 2026 10:19
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 29, 2026 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.65%. Comparing base (2a601eb) to head (c70ec6d).
⚠️ Report is 11 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #63032      +/-   ##
==========================================
- Coverage   89.65%   89.65%   -0.01%     
==========================================
  Files         708      708              
  Lines      220374   220374              
  Branches    42264    42274      +10     
==========================================
- Hits       197584   197577       -7     
- Misses      14656    14663       +7     
  Partials     8134     8134
Files with missing lines Coverage Δ
lib/internal/webstreams/util.js 99.56% <100.00%> (ø)

... and 32 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Apr 29, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 29, 2026
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Apr 29, 2026

CI: https://ci.nodejs.org/job/node-test-pull-request/73020/

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Apr 29, 2026

CI: https://ci.nodejs.org/job/node-test-pull-request/73026/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@targos targos targos approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. web streams

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@aduh95 @targos @nodejs-github-bot